-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes for bug reported under MSVC Debug #175
Conversation
Thanks you very much for inspecting this. I'm confused about the alphabet parts, these should match into unsigned char for everything but national - and in this case we shouldn't cast to unsigned char... The picture was indeed a bug, please add U in the precedence table before N. |
At the point where the error is reported (
The seems to match the changes made by SVN commit 5310 to test We probably need to have national-specific variants of |
Yes, we need national variables instead of masking that error.
Is the digit one similar?
Am 26. August 2024 00:42:15 MESZ schrieb OCP David Declerck ***@***.***>:
…> I'm confused about the alphabet parts, these should match into unsigned char for everything but national - and in this case we shouldn't cast to unsigned char... Similar for the digits. Can you elaborate on where this happens? I guess there's a different bug to solve there...
At the point where the error is reported (`typeck.c:3825`, function `cb_validate_collating`), the node contains the following info:
![Capture d’écran du 2024-08-26 00-29-07](https://github.com/user-attachments/assets/01a943f7-ab1b-4ef7-87a9-6abfba09abef)
`high_val_char` is indeed outside the range of an `unsigned char`.
The seems to match the changes made by SVN commit [5310](https://sourceforge.net/p/gnucobol/code/5310) to test `PROGRAM COLLATING SEQUENCE` in `syn_definition.at`.
We probably need to have national-specific variants of `cb_low` and `cb_high`.
This could also be an opportunity to have a look at #136, which deals with these HIGH/LOW-VALUES as well.
--
Reply to this email directly or view it on GitHub:
#175 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
I adjusted the precedence table as needed. For the digit case, this is really no big deal.
Since bit-shifting promotes to
Same reason as above, so I added extra parentheses so that the masking occurs before the cast.
As this seems a bit more involved than a mere fixup, should I make a dedicated PR for this (or why not, include that in #136) ? |
As 136 is not about national - please do an extra one (and if not done yet, drop that part from this PR).
I'll try to recheck this and the merge4 bunch during our longer lunch break (I'm on "holiday" - doing construction work - this week).
Thank you for working on this!
|
Dropped the alphabet parts from this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please have a look at the seed part, then feel free to commit directly.
Is there anything wrong with my proposed solution ? |
I don't think that The idea is to always define the seed as cob_u64t - possibly better |
Isn't |
The functions we call use unsigned long or int. And yes, that's "fun" with the different sizes.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for upstream.
Hosted should work though as it should compile the C files using cobc which will use msvc, no? If that doesn't happen then an adjustment of the commands used should do the trick.
The thing is, even if libcob uses the debug C runtime, cobc will only link with that runtime if given the Would it make sense (at least for MSVC), to always link generated executables/modules with the same version of the C runtime as the one used by cobc/libcob ? |
No, as this would mean a necessary relink by switching that. For now please add your note for both "still skipped" test in the CI definition (maybe just in #170) and the changes to GnuCOBOL from this one as-is upstream. |
Merged in SVN @ 5318. |
This PR fixes problems reported by the runtime error checker under MSVC Debug.
Most of them were fixed by being pedantic about casts with loss of data (as hinted by the error popups).
One of them (CURRENCY SIGN WITH PICTURE SYMBOL) was more serious: a stack corruption occurring because we were writing outside of an array. Actually SVN commit 4886 added a picture string symbol (U)but did not extend the relevant tables. I added one to the table size (cf
tree.c
), which is sufficient to avoid the corruption, however I'm not sure what data I should add to theprecedence_table
for this new picture string symbol (the patch itself only claims to add "minimal parsing support", so maybe it's just sufficient ?).Only one issue remain with test "System routine CBL_GC_HOSTED", which fails because of a mismatch in the C runtime used by libcob and by custom C code (as the test itself claims: "test_stdio.c must be compiled with same C runtime as libcob to match").